-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify the lightingShader to allow per-vertex coloring #5938
Modify the lightingShader to allow per-vertex coloring #5938
Conversation
Prepare a flag that determines whether the array that stores the color for each vertex contains an element. Also, for retainedMode, prepare a shader for simply coloring with vertex colors only.
Computes a flag that determines whether to do per-vertex coloring in immediateMode. This will always be true, but just in case we do the math.
Computes a flag for per-vertex coloring in retainedMode. This will be false for drawing primitives, for example. It becomes true when color information is stored in vertexColors such as drawing with p5.Geometry. This flag is used to retrieve the shader, so it must be calculated ahead of time.
Instead of uMaterialColor, we calculate vColor with vertex shader and use the value passed by varying.
Instead of uMaterialColor, we calculate vColor with vertex shader and use the value passed by varying.
Determines whether to use per-vertex color or solid color depending on whether uUseVertexColor is ON/OFF
Determines whether to use per-vertex color or solid color depending on whether uUseVertexColor is ON/OFF
I don't know what went wrong... |
Looks like the tests timed out. If you run |
Oh never mind, I don't think it's you, I think it was caused by this dependabot change: #5922 I'm going to see if reverting it causes tests to pass again |
Ok looks like that fixes it for now. #5939 isn't merged into main yet but in the mean time you can run |
Thank you for your reply! Ok, I'll wait until the version comes back before doing that. Also, I'm thinking of rewriting the code in the direction of rewriting the shader instead of creating a new shader. |
Instead of having a new shader, I decided to rewrite the existing shader.
Instead of uMaterialColor, we calculate vColor with vertexShader and use the received value.
Computes vColor, the color variable passed to basicFrag, in normalVert, the vertexShader used for solid color drawing in retainedMode. This will draw based on per-vertex colors if vertexColors contains colors.
I created 6 unit tests... First, tests that vertex colors are valid under the lighting shader in immediate mode. test('immediate mode uses vertex colors (noLight)', function(done) {
const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);
// upper color: (200, 0, 0, 255);
// lower color: (0, 0, 200, 255);
// expected center color: (100, 0, 100, 255);
myp5.beginShape();
myp5.fill(200, 0, 0);
myp5.vertex(-128, -128);
myp5.fill(200, 0, 0);
myp5.vertex(128, -128);
myp5.fill(0, 0, 200);
myp5.vertex(128, 128);
myp5.fill(0, 0, 200);
myp5.vertex(-128, 128);
myp5.endShape(myp5.CLOSE);
assert.equal(renderer._useVertexColor, true);
assert.deepEqual(myp5.get(128, 128), [100, 0, 100, 255]);
done();
});
test('immediate mode uses vertex colors (light)', function(done) {
const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);
myp5.directionalLight(255, 255, 255, 0, 0, -1);
// diffuseFactor:0.73
// so, expected color is (73, 0, 73, 255).
myp5.beginShape();
myp5.fill(200, 0, 0);
myp5.vertex(-128, -128);
myp5.fill(200, 0, 0);
myp5.vertex(128, -128);
myp5.fill(0, 0, 200);
myp5.vertex(128, 128);
myp5.fill(0, 0, 200);
myp5.vertex(-128, 128);
myp5.endShape(myp5.CLOSE);
assert.equal(renderer._useVertexColor, true);
assert.deepEqual(myp5.get(128, 128), [73, 0, 73, 255]);
done();
}); Second, if you don't use vertex colors in retained mode, make sure _useVertexColor is off. test('geom without vertex colors use curFillCol (noLight)', function(done) {
const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);
// expected center color is curFillColor.
myp5.fill(200, 0, 200);
myp5.rectMode(myp5.CENTER);
myp5.rect(0, 0, myp5.width, myp5.height);
assert.equal(renderer._useVertexColor, false);
assert.deepEqual(myp5.get(128, 128), [200, 0, 200, 255]);
done();
});
test('geom without vertex colors use curFillCol (light)', function(done) {
const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);
myp5.directionalLight(255, 255, 255, 0, 0, -1);
// diffuseFactor:0.73
// so, expected color is (146, 0, 146, 255).
myp5.fill(200, 0, 200);
myp5.rectMode(myp5.CENTER);
myp5.rect(0, 0, myp5.width, myp5.height);
assert.equal(renderer._useVertexColor, false);
assert.deepEqual(myp5.get(128, 128), [146, 0, 146, 255]);
done();
}); Finally, Make sure vertex colors are reflected in the drawing when enabled in retained mode (even if the lighting shader is working). test('geom with vertex colors use their color (noLight)', function(done) {
const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);
// upper color: (200, 0, 0, 255);
// lower color: (0, 0, 200, 255);
// expected center color: (100, 0, 100, 255);
const myGeom = new p5.Geometry(1, 1, function() {
this.gid = 'vertexColorTestWithNoLights';
this.vertices.push(myp5.createVector(-128, -128));
this.vertices.push(myp5.createVector(128, -128));
this.vertices.push(myp5.createVector(128, 128));
this.vertices.push(myp5.createVector(-128, 128));
this.faces.push([0, 1, 2]);
this.faces.push([0, 2, 3]);
this.vertexColors.push(
200/255, 0, 0, 1,
200/255, 0, 0, 1,
0, 0, 200/255, 1,
0, 0, 200/255, 1
);
this.computeNormals();
});
myp5.noStroke();
myp5.model(myGeom);
assert.equal(renderer._useVertexColor, true);
assert.deepEqual(myp5.get(128, 128), [100, 0, 100, 255]);
done();
});
test('geom with vertex colors use their color (light)', function(done) {
const renderer = myp5.createCanvas(256, 256, myp5.WEBGL);
const myGeom = new p5.Geometry(1, 1, function() {
this.gid = 'vertexColorTestWithLighs';
this.vertices.push(myp5.createVector(-128, -128));
this.vertices.push(myp5.createVector(128, -128));
this.vertices.push(myp5.createVector(128, 128));
this.vertices.push(myp5.createVector(-128, 128));
this.faces.push([0, 1, 2]);
this.faces.push([0, 2, 3]);
this.vertexColors.push(
200/255, 0, 0, 1,
200/255, 0, 0, 1,
0, 0, 200/255, 1,
0, 0, 200/255, 1
);
this.computeNormals();
});
myp5.directionalLight(255, 255, 255, 0, 0, -1);
// diffuseFactor:0.73
// so, expected color is (73, 0, 73, 255).
myp5.noStroke();
myp5.model(myGeom);
assert.equal(renderer._useVertexColor, true);
assert.deepEqual(myp5.get(128, 128), [73, 0, 73, 255]);
done();
}); However, even if I commit now, I will not be able to pass the checks, so I would like to wait for the situation to be resolved. |
Thank you both @inaridarkfox4231 and @davepagurek. I restarted the check process, and it's all passed. Ready to merge this PR if it looks good to you both. |
@Qianqianye Thank you for responding! Since the unit test is not yet done, I think that it will be merged after adding it. |
Added some tests to make sure that vertex color interpolation in immediate mode and retained mode performs properly even when lighting shaders are enabled.
erase several trailing spaces.
There are many small mistakes...sorry. |
Instead of writing "myp5.createCanvas", I wrote "createCanvas" directly. So I fixed it.
When I stopped modifying the code, it ended up with one line of space omitted, so I put it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks great! Seems like a very natural extension of the current system, and with detailed tests. Great work!
Thanks very much! ('ω') I'm glad I did my best. |
Currently, if you try to do per-vertex coloring, for example in immediateMode, using functions like pointLight won't enable it.
By modifying the shader, we will be able to achieve both coloring for each vertex and lighting processing by the shader.
Also, even in retainedMode, if vertexColors contains a color element, it will be possible to color each vertex.
Resolves #5936
Changes
Source code:
Screenshots of the change:
PR Checklist